Skip to content

Catch invalid JSON response and raise Mollie::RequestError #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

ppostma
Copy link
Contributor

@ppostma ppostma commented Dec 20, 2024

This PR tries to handle invalid JSON responses (for e.g. 500 / 503 errors with HTML body) and raise a Mollie::RequestError instead.

Occasionally, we see these errors in our logs, and it would be nice to be able to handle this in a clean way.
We are already handling Mollie::RequestError in our application and I think it makes sense to raise that error for this scenario too.

Must admit that it's an edge case, but it would still be nice to handle it like this and it would make our logs cleaner as well.

@fjbender
Copy link
Contributor

I appreciate your contribution!

We have, however, agreed internally not to merge this into the main line of the Client. We'll be fixing things so that those messages in HTML should appear a lot less frequently, and for 500/502/503 types of errors proper, well-formed JSON will be returned. And if they do appear, we wouldn't want the Client to raise a Mollie::RequestError - these messages could very well also come from a proxy in the client application's chain, so we deliberately choose to be noisy about it.

Again, many thanks for your contribution - please keep the feedback coming!

In case you didn't know it yet, please consider joining our Developer Discord at https://discord.gg/mollie where I'll be available alongside many other Mollie team members to discuss this further :)

@fjbender fjbender closed this Dec 20, 2024
@ppostma
Copy link
Contributor Author

ppostma commented Dec 20, 2024

Thanks for your quick reply.

Makes a lot of sense what you say, so I completely understand this choice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants